perf: optimize generated native dispatch#33
Conversation
55e9189 to
de6be51
Compare
|
Generated by Codex. Current head: Suggested human review path:
Local verification run on this head:
CI for |
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds engine-direct dispatch and per-engine fast-native paths, extends signature dispatch generation, refactors ObjC bridge caches/threading, installs Hermes JSI, introduces React Native TurboModule and Fabric view, updates CI to publish new targets, and includes demos and an Objective-C dispatch benchmark suite. ChangesEnd-to-end engine-direct and RN integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
NativeScript/ffi/Class.mm (1)
602-610:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove narrowing conversions in fast-enumeration bridge.
Line 605 and Line 609 currently rely on implicit
NSUIntegernarrowing. It works withcount:16, but it keeps CI warning noise and can hide future widening regressions.Suggested fix
for (NSUInteger index = 0; index < count; index++) { id obj = self->state.itemsPtr[index]; napi_value jsObj = bridgeState->getObject(env, obj); - napi_set_element(env, stackArray, index, jsObj); + napi_set_element(env, stackArray, static_cast<uint32_t>(index), jsObj); } napi_value result; -napi_create_int32(env, count, &result); +napi_create_uint32(env, static_cast<uint32_t>(count), &result);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/Class.mm` around lines 602 - 610, The loop currently uses NSUInteger and implicitly narrows to uint32_t when calling napi_set_element and napi_create_int32, causing warnings; change the loop index to uint32_t (e.g., for (uint32_t index = 0; index < (uint32_t)count; ++index) ), keep using self->state.itemsPtr[index] and bridgeState->getObject(env, obj) as before, use napi_set_element(env, stackArray, index, jsObj) with the uint32_t index, and replace napi_create_int32(env, count, &result) with napi_create_uint32(env, (uint32_t)count, &result) to avoid narrowing conversions.NativeScript/ffi/CFunction.mm (1)
342-349:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAvoid shared
Cifscratch buffers while the runtime is unlocked.This path still marshals through
cif->avalues/cif->rvalue, but the native call now runs underNativeCallRuntimeUnlockScope. A synchronous native callback can re-enter JS and invoke the sameCFunctionbefore the outer call finishes, clobbering those shared buffers and corrupting the outer invocation.Also applies to: 378-389, 405-417, 449-489
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/CFunction.mm` around lines 342 - 349, The code currently marshals arguments/results through the shared Cif scratch buffers (cif->avalues / cif->rvalue) while executing the native call under NativeCallRuntimeUnlockScope, which allows a nested re-entry to clobber those buffers; change the call sites that use CFunction::cif (and functions like ensureCFunctionDispatchLookup, preparedInvoker, napiInvoker, engineDirectInvoker) to copy needed avalues and rvalue into per-invocation scratch storage before unlocking the runtime (e.g., allocate small stack/local arrays or a temporary vector for the arg/value pointers), then pass those per-call buffers to the invoker and restore any needed state after the call — ensure every path that currently references cif->avalues or cif->rvalue (including the other noted blocks) is updated to use these per-invocation copies so the shared cif buffers are never used while the runtime is unlocked.
🧹 Nitpick comments (3)
NativeScript/ffi/JSCFastNativeApi.h (1)
6-16: ⚡ Quick winGuard
namespace nativescriptinNativeScript/ffi/JSCFastNativeApi.hwith__cplusplus
JSCFastNativeApi.his currently included only from C++/ObjC++ units (NativeScript/ffi/JSCFastNativeApi.mm,NativeScript/napi/jsc/jsc-api.cpp), and no.cfiles reference it directly; however, the unguardednamespacewould break immediately if this header ever becomes reachable from a C translation unit.Proposed fix
`#include` "js_native_api.h" -namespace nativescript { +#ifdef __cplusplus +namespace nativescript { +#endif `#ifdef` TARGET_ENGINE_JSC bool JSCTryDefineFastNativeProperty(napi_env env, napi_value object, const napi_property_descriptor* descriptor); `#endif` // TARGET_ENGINE_JSC -} // namespace nativescript +#ifdef __cplusplus +} // namespace nativescript +#endif🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/JSCFastNativeApi.h` around lines 6 - 16, Wrap the namespace block with `#ifdef` __cplusplus / `#endif` so the identifier namespace nativescript { ... } (containing JSCTryDefineFastNativeProperty) is only emitted for C++/ObjC++ compiles; i.e., surround the opening "namespace nativescript {" and the corresponding closing " } // namespace nativescript" with `#ifdef` __cplusplus and `#endif` to prevent a C translation unit from seeing an unguarded C++ namespace.NativeScript/ffi/HermesFastCallbackInfo.h (1)
54-57: 💤 Low valueMissing null check before dereferencing.
HermesFastThisArgdoes not validateinfoorinfo->framebefore dereferencing, unlike the other accessor functions. If a caller bypassesTryGetHermesFastCallbackInfo, this could cause a crash.Proposed defensive check
inline napi_value HermesFastThisArg(const HermesFastCallbackInfo* info) { + if (info == nullptr || info->frame == nullptr) { + return nullptr; + } return reinterpret_cast<napi_value>( const_cast<uint64_t*>(info->frame->thisArgAndArgsBase)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/HermesFastCallbackInfo.h` around lines 54 - 57, HermesFastThisArg currently dereferences info and info->frame without checks and can crash if a caller bypasses TryGetHermesFastCallbackInfo; add the same defensive null-checks used elsewhere: verify info is non-null and info->frame is non-null before accessing info->frame->thisArgAndArgsBase, and return nullptr (or the function's appropriate null napi_value) when the checks fail; update HermesFastThisArg to mirror the validation pattern of the other accessor functions and keep the return type/behavior consistent with callers that expect a null napi_value.metadata-generator/src/SignatureDispatchEmitter.cpp (1)
654-729: 💤 Low valueDocument the partial-code-generation contract.
writeHermesDirectReturnValuegenerates an openifblock that the caller must close. This pattern appears intentional but is non-obvious. A brief comment at the function declaration explaining that the caller is responsible for emitting the cleanup logic,return false, and closing brace would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metadata-generator/src/SignatureDispatchEmitter.cpp` around lines 654 - 729, The function writeHermesDirectReturnValue currently emits an open "if" block that callers must close, but this contract is undocumented; add a short comment above the writeHermesDirectReturnValue declaration stating that the function writes the opening of an if(...) { block and that the caller is responsible for emitting the corresponding cleanup logic, the "return false;" (on failure), and the closing brace '}' so callers like whoever invokes writeHermesDirectReturnValue know to finish the block correctly; reference the symbol writeHermesDirectReturnValue and mention the related concepts "open if block", "cleanup logic", "return false", and "closing brace" in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/npm_trusted_release.yml:
- Around line 123-126: The workflow is unsafe because it interpolates
workflow_dispatch inputs directly into the run script (e.g., release_type='${{
inputs.release-type }}', preid='${{ inputs.preid }}', target='${{ matrix.target
}}'), allowing shell injection; change the step to pass these values via env:
(e.g., set environment variables like RELEASE_TYPE, PREID, TARGET) and then
reference them inside the run body as quoted shell variables ("$RELEASE_TYPE",
"$PREID", "$TARGET"); apply the same env + "$VAR" quoting pattern to every run
step that currently splices `${{ inputs.* }}` (including the summary step) and
update references to the functions/variables release_type, preid, and target
accordingly.
- Around line 57-84: Add explicit least-privilege GITHUB_TOKEN permissions for
the affected jobs by declaring permissions: contents: read (and id-token: write
only where needed) at the job-level for the matrix, build, and summary jobs
(instead of relying on repo defaults) so each job's token scope is minimal; also
fix the command-injection risk in the build job where inputs.preid is inlined by
moving inputs.preid into the job environment (e.g., env: PREID: ${{ inputs.preid
}}) and reference that env var inside the bash step (or otherwise properly
escape it) rather than embedding ${{ inputs.preid }} into the shell script body.
In `@benchmarks/objc-dispatch/run.js`:
- Around line 611-622: When the child process exits without emitting the
benchmark marker the code currently just waits for the timeout; update the
child.on("exit", ...) handler to fail fast: if !settled and output does NOT
include marker, create a failure report that includes the exit code and the
child's stderr (use the same captured stderr buffer/variable you already
collect) and immediately call the settling path (e.g. settleWithReport or the
equivalent settle/error function) instead of doing nothing; reference the
symbols child.on("exit", settled, output, marker, settleWithReport,
parseBenchmarkOutput) so you locate and replace the current no-op branch with an
immediate failure/settle containing the stderr and exit code.
- Around line 460-462: Replace the ad-hoc file URL constructor in pathToFileUrl
with Node's pathToFileURL: import and return pathToFileURL(filePath).href inside
the existing pathToFileUrl function so dynamic import works for paths with #,
spaces, etc.; and update launchAndCollect's simctl launch exit handler to fail
fast by rejecting when the child process exits with a non-zero code (include
stderr/stdout or the exit code in the rejection), and ensure you remove
listeners/kill or cleanup the child process and resolve/reject only once to
avoid waiting for the marker after a launch failure.
In `@examples/react-native-demo/App.tsx`:
- Around line 47-50: Replace the direct use of
UIApplication.sharedApplication.keyWindow with a scene-aware lookup: when
obtaining the window (currently using UIApplication.sharedApplication and
keyWindow), enumerate UIApplication.sharedApplication.connectedScenes, find the
UIWindowScene whose activationState is .foregroundActive (or
.foregroundForegroundActive equivalent) and then pick its windows array entry
with isKeyWindow == true; if none found, fall back to the existing keyWindow
(for iOS 12 and earlier). Update the code that throws "No key UIWindow is
available yet" to use this scene-aware window variable (referencing
UIApplication.sharedApplication, connectedScenes, UIWindowScene.activationState,
windows, and isKeyWindow) so it only throws when no window can be resolved after
the scene-aware fallback.
In `@NativeScript/ffi/HermesFastNativeApi.mm`:
- Around line 2352-2380: The C-function invocation writes into the cached
Cif->rvalue while the runtime is unlocked, risking clobbering across
concurrent/recursive calls; allocate a per-call return buffer (local stack or
heap) and pass its pointer instead of cif->rvalue into
ensureHermesCFunctionEngineDirectInvoker/InvokeCFunctionEngineDirectDynamic (use
a local variable like perCallRValue), then after the invocation (while still in
this function, before returning) convert that per-call buffer by calling
makeHermesCFunctionReturnValue or copying it into the cif if needed; update the
call sites that currently pass cif->rvalue to pass perCallRValue and ensure
makeHermesCFunctionReturnValue(env, function, cif, perCallRValue) or equivalent
is used so the cached Cif->rvalue is no longer written while runtime is
unlocked.
- Around line 1639-1650: The branch that boxes Hermes booleans/numbers for every
non-class object kind incorrectly creates NSNumbers for NSString fast args;
modify the condition inside the non-mdTypeClass branch so it first checks
whether kind is mdTypeNSStringObject or mdTypeNSMutableStringObject and, if so,
performs the normal string conversion (instead of boxing) into
*reinterpret_cast<id*>(result); otherwise continue to handle isHermesBool and
isHermesNumber as before. Update the logic near hermesRawValueBits(value),
isHermesBool, isHermesNumber and the assignment to
*reinterpret_cast<id*>(result) to skip NSNumber boxing for the two NSString
kinds.
- Around line 202-233: The mutableString branch currently allocates an
NSMutableString via initWithCharacters: and returns a retained object (leak);
change it to return an autoreleased instance so callers don’t leak: after
creating the string with [[NSMutableString alloc] initWithCharacters:...
length:utf16Length] call autorelease (or replace with a convenience constructor
such as +stringWithCharacters:length:), assign that autoreleased object to
*result and return true; update the allocation site in HermesFastNativeApi.mm
(the mutableString branch and the expression using initWithCharacters)
accordingly.
- Around line 1596-1625: Both TryFastConvertHermesInt64Argument and
TryFastConvertHermesUInt64Argument accept BigInts even when
napi_get_value_bigint_int64/_uint64 returns lossless==false, which can truncate
values; update each function to call napi_get_value_bigint_int64 /
napi_get_value_bigint_uint64 into the existing result and lossless variables and
only return true if the call returns napi_ok AND lossless is true (otherwise
return false) so oversized BigInt arguments are rejected on the Hermes fast
path.
In `@NativeScript/ffi/JSCFastNativeApi.mm`:
- Around line 184-186: The mdTypeVoid branch currently returns
JSValueMakeNull(ctx) which yields null; change both occurrences of the
mdTypeVoid case in JSCFastNativeApi.mm (the fast-path switch handling return
types — see the case mdTypeVoid near the top and the duplicate at ~1776-1778) to
return JSValueMakeUndefined(ctx) and return true so void native calls produce
undefined, matching other backends.
- Around line 879-920: The code can set env->last_exception during a speculative
direct call even when that call returns false, which leaks that exception into
the subsequent slow-path; after each direct attempt (the calls to
tryCallJSCObjCEngineDirect and tryCallJSCCFunctionEngineDirect) ensure you only
preserve env->last_exception when didUseDirectResult is true—if
didUseDirectResult is false, clear env->last_exception (set to nullptr)
immediately so the slow-path isn't given a spurious pending exception; update
the blocks around the tryCallJSCObjCEngineDirect and
tryCallJSCCFunctionEngineDirect invocations to reset env->last_exception when
the direct path wasn't used.
In `@NativeScript/ffi/ObjCBridge.h`:
- Around line 215-233: In cacheHandleObjectRef, the call
deleteOwnedHandleObjectRef(env, it->second) uses an undefined env; change the
call to the existing overload that doesn't require env by calling
deleteOwnedHandleObjectRef(it->second) (or update cacheHandleObjectRef signature
to accept an env if that overload is required), and keep the rest of the logic
around handleObjectRefs, bumpHandleObjectRefsGeneration(), and HandleObjectRef
unchanged.
In `@NativeScript/ffi/QuickJSFastNativeApi.mm`:
- Around line 275-277: The fast-path currently maps mdTypeVoid to JS_NULL (see
mdTypeVoid case in QuickJSFastNativeApi.mm) causing void-return natives to
surface as null; update the case handling to return JS_UNDEFINED instead of
JS_NULL and ensure the same replacement is applied to the other identical
mdTypeVoid branch later in the file (the second occurrence around the longer
native-call handling) so both fast-paths consistently produce undefined for void
results.
- Around line 992-1094: A pending QuickJS exception from
tryCallQuickJS*EngineDirect can remain and be rethrown after the slow path;
before entering the slow path (i.e., after the didUseDirectReturn branch and
before QuickJSFastStackHandleScope) clear any pending exception by calling
JS_GetException(context) and JS_FreeValue(context, <that exception>) (or
equivalent QuickJS APIs) so the slow-path result isn't shadowed by a stale
exception; reference tryCallQuickJSObjCEngineDirect,
tryCallQuickJSCFunctionEngineDirect, QuickJSFastStackHandleScope,
JS_HasException, JS_GetException, and JS_FreeValue to locate the correct place
to insert the clear.
In `@NativeScript/ffi/SignatureDispatch.h`:
- Around line 139-163: The helpers readHermesDispatchFiniteNumber and
readHermesDispatchFiniteNumberRaw must reject non-finite Hermes numbers instead
of coercing to 0.0: change the logic so that after confirming
isHermesDispatchNumber(raw) you check hermesDispatchRawDoubleIsFinite(raw) and
return false if it's not finite; only call hermesDispatchRawToDouble(raw) and
store into *result when the value is finite, and ensure you still return true on
success and false on any null-pointer or non-finite condition.
In `@NativeScript/ffi/TypeConv.mm`:
- Around line 2200-2213: The direct-lookup fallback is gated by "if
(objNormalized != objPtr)" so it only runs when the incoming pointer has extra
bits; instead remove that gate and always iterate
bridgeState->mdProtocolsByPointer when the direct lookup misses: compute objPtr
and objNormalized as before, then unconditionally loop over
bridgeState->mdProtocolsByPointer, compare normalizePtr((void*)entry.first) to
objNormalized, call bridgeState->getProtocol(env, entry.second) and if proto !=
nullptr return get_ref_value(env, proto->constructor); this ensures
metadata-side keys with tag/auth bits are matched even when the incoming pointer
was already normalized.
In `@packages/react-native/ios/Fabric/NativeScriptUIViewComponentView.mm`:
- Around line 15-17: The +load override in NativeScriptUIViewComponentView (the
+load method in NativeScriptUIViewComponentView.mm) should not call [super load]
manually; remove the explicit [super load] call (or remove the entire +load
override if it does nothing) so the Objective-C runtime handles superclass +load
invocation and avoids double-invoking superclass load-time side effects.
---
Outside diff comments:
In `@NativeScript/ffi/CFunction.mm`:
- Around line 342-349: The code currently marshals arguments/results through the
shared Cif scratch buffers (cif->avalues / cif->rvalue) while executing the
native call under NativeCallRuntimeUnlockScope, which allows a nested re-entry
to clobber those buffers; change the call sites that use CFunction::cif (and
functions like ensureCFunctionDispatchLookup, preparedInvoker, napiInvoker,
engineDirectInvoker) to copy needed avalues and rvalue into per-invocation
scratch storage before unlocking the runtime (e.g., allocate small stack/local
arrays or a temporary vector for the arg/value pointers), then pass those
per-call buffers to the invoker and restore any needed state after the call —
ensure every path that currently references cif->avalues or cif->rvalue
(including the other noted blocks) is updated to use these per-invocation copies
so the shared cif buffers are never used while the runtime is unlocked.
In `@NativeScript/ffi/Class.mm`:
- Around line 602-610: The loop currently uses NSUInteger and implicitly narrows
to uint32_t when calling napi_set_element and napi_create_int32, causing
warnings; change the loop index to uint32_t (e.g., for (uint32_t index = 0;
index < (uint32_t)count; ++index) ), keep using self->state.itemsPtr[index] and
bridgeState->getObject(env, obj) as before, use napi_set_element(env,
stackArray, index, jsObj) with the uint32_t index, and replace
napi_create_int32(env, count, &result) with napi_create_uint32(env,
(uint32_t)count, &result) to avoid narrowing conversions.
---
Nitpick comments:
In `@metadata-generator/src/SignatureDispatchEmitter.cpp`:
- Around line 654-729: The function writeHermesDirectReturnValue currently emits
an open "if" block that callers must close, but this contract is undocumented;
add a short comment above the writeHermesDirectReturnValue declaration stating
that the function writes the opening of an if(...) { block and that the caller
is responsible for emitting the corresponding cleanup logic, the "return false;"
(on failure), and the closing brace '}' so callers like whoever invokes
writeHermesDirectReturnValue know to finish the block correctly; reference the
symbol writeHermesDirectReturnValue and mention the related concepts "open if
block", "cleanup logic", "return false", and "closing brace" in the comment.
In `@NativeScript/ffi/HermesFastCallbackInfo.h`:
- Around line 54-57: HermesFastThisArg currently dereferences info and
info->frame without checks and can crash if a caller bypasses
TryGetHermesFastCallbackInfo; add the same defensive null-checks used elsewhere:
verify info is non-null and info->frame is non-null before accessing
info->frame->thisArgAndArgsBase, and return nullptr (or the function's
appropriate null napi_value) when the checks fail; update HermesFastThisArg to
mirror the validation pattern of the other accessor functions and keep the
return type/behavior consistent with callers that expect a null napi_value.
In `@NativeScript/ffi/JSCFastNativeApi.h`:
- Around line 6-16: Wrap the namespace block with `#ifdef` __cplusplus / `#endif` so
the identifier namespace nativescript { ... } (containing
JSCTryDefineFastNativeProperty) is only emitted for C++/ObjC++ compiles; i.e.,
surround the opening "namespace nativescript {" and the corresponding closing "
} // namespace nativescript" with `#ifdef` __cplusplus and `#endif` to prevent a C
translation unit from seeing an unguarded C++ namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2275070d-371f-4b9f-b586-4b63e0f8b47a
📒 Files selected for processing (102)
.github/workflows/npm_trusted_release.yml.gitignoreNativeScript/CMakeLists.txtNativeScript/NativeScript-Prefix.pchNativeScript/ffi/Block.hNativeScript/ffi/Block.mmNativeScript/ffi/CFunction.hNativeScript/ffi/CFunction.mmNativeScript/ffi/CallbackThreading.hNativeScript/ffi/Cif.hNativeScript/ffi/Cif.mmNativeScript/ffi/Class.mmNativeScript/ffi/ClassBuilder.mmNativeScript/ffi/ClassMember.hNativeScript/ffi/ClassMember.mmNativeScript/ffi/Closure.mmNativeScript/ffi/EngineDirectCall.hNativeScript/ffi/EngineDirectCall.mmNativeScript/ffi/HermesFastCallbackInfo.hNativeScript/ffi/HermesFastNativeApi.hNativeScript/ffi/HermesFastNativeApi.mmNativeScript/ffi/Interop.mmNativeScript/ffi/JSCFastNativeApi.hNativeScript/ffi/JSCFastNativeApi.mmNativeScript/ffi/ObjCBridge.hNativeScript/ffi/ObjCBridge.mmNativeScript/ffi/Object.hNativeScript/ffi/Object.mmNativeScript/ffi/Protocol.mmNativeScript/ffi/QuickJSFastNativeApi.hNativeScript/ffi/QuickJSFastNativeApi.mmNativeScript/ffi/SignatureDispatch.hNativeScript/ffi/TypeConv.hNativeScript/ffi/TypeConv.mmNativeScript/ffi/V8FastNativeApi.hNativeScript/ffi/V8FastNativeApi.mmNativeScript/ffi/jsi/NativeApiJsi.hNativeScript/ffi/jsi/NativeApiJsi.mmNativeScript/ffi/jsi/NativeApiJsiReactNative.hNativeScript/ffi/jsi/README.mdNativeScript/napi/hermes/jsr.cppNativeScript/napi/hermes/jsr.hNativeScript/napi/jsc/jsc-api.cppNativeScript/napi/jsc/jsc-api.hNativeScript/napi/quickjs/quickjs-api.cNativeScript/napi/v8/v8-api.cppNativeScript/runtime/Runtime.cppNativeScript/runtime/modules/console/Console.cppNativeScript/runtime/modules/node/Process.cppNativeScript/runtime/modules/timers/Timers.mmbenchmarks/objc-dispatch/README.mdbenchmarks/objc-dispatch/objc-dispatch-benchmarks.jsbenchmarks/objc-dispatch/run.jsexamples/expo-demo/App.tsxexamples/expo-demo/README.mdexamples/expo-demo/app.config.jsexamples/react-native-demo/App.tsxexamples/react-native-demo/README.mdmetadata-generator/src/SignatureDispatchEmitter.cpppackage.jsonpackages/ios/package.jsonpackages/react-native/LICENSEpackages/react-native/NativeScriptNativeApi.podspecpackages/react-native/README.mdpackages/react-native/app.plugin.jspackages/react-native/ios/Fabric/NativeScriptUIViewComponentView.hpackages/react-native/ios/Fabric/NativeScriptUIViewComponentView.mmpackages/react-native/ios/NativeScriptNativeApiModule.hpackages/react-native/ios/NativeScriptNativeApiModule.mmpackages/react-native/ios/NativeScriptNativeApiModuleProvider.hpackages/react-native/ios/NativeScriptNativeApiModuleProvider.mmpackages/react-native/ios/NativeScriptUIView.hpackages/react-native/ios/NativeScriptUIView.mmpackages/react-native/ios/NativeScriptUIViewManager.mmpackages/react-native/package.jsonpackages/react-native/plugin/withNativeScriptReactNative.jspackages/react-native/react-native.config.jspackages/react-native/src/NativeScriptNativeApi.tspackages/react-native/src/NativeScriptUIViewNativeComponent.tspackages/react-native/src/index.d.tspackages/react-native/src/index.tsscripts/build_all_ios.shscripts/build_all_macos.shscripts/build_all_node_api.shscripts/build_nativescript.shscripts/build_npm_ios.shscripts/build_react_native_turbomodule.shscripts/create_react_native_demo.shscripts/get-npm-tag.jsscripts/metagen.jsscripts/react_native_app_utils.shscripts/run-tests-macos.jsscripts/test_react_native_ffi_compat.shscripts/test_react_native_turbomodule.shtest/react-native/ffi-compat/App.tsxtest/runtime/fixtures/Marshalling/TNSFunctionPointers.htest/runtime/fixtures/Marshalling/TNSFunctionPointers.mtest/runtime/fixtures/exported-symbols.txttest/runtime/runner/app/tests/ApiTests.jstest/runtime/runner/app/tests/Marshalling/FunctionPointerTests.jstest/runtime/runner/app/tests/NativeApiJsiTests.jstest/runtime/runner/app/tests/index.js
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
NativeScript/ffi/TypeConv.mm (1)
2823-2839:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep a pointer fallback when the bridge state is unavailable.
For a non-null
Class, this now returnsnullptrifObjCBridgeState::InstanceData(env)is missing. That propagates an invalidnapi_valueinstead of the existing best-effort handle behavior used elsewhere in this file.Suggested change
- auto bridgeState = ObjCBridgeState::InstanceData(env); - return bridgeState != nullptr ? bridgeState->getObject(env, (id)cls, kUnownedObject, 0, nullptr) - : nullptr; + auto bridgeState = ObjCBridgeState::InstanceData(env); + if (bridgeState != nullptr) { + return bridgeState->getObject(env, (id)cls, kUnownedObject, 0, nullptr); + } + return Pointer::create(env, (void*)cls);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/TypeConv.mm` around lines 2823 - 2839, The toJS override currently returns nullptr when ObjCBridgeState::InstanceData(env) is missing, which yields an invalid napi_value; instead, keep the pointer fallback for non-null Class values: in toJS (the function containing Class cls, the findRegisteredClassConstructor call, and the ObjCBridgeState::InstanceData(env) lookup), if bridgeState is null call the same pointer-to-JS fallback used elsewhere in this file (i.e., construct a valid napi_value representing the raw Class pointer or external handle) and return that rather than nullptr so callers receive a valid JS handle when getObject cannot be used.NativeScript/ffi/ObjCBridge.mm (1)
978-997:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't bypass the proxy for large
NSMutableArrayinstances.
nativeIsMutableArraycurrently forces the fast-array path even when the array is larger thankHermesFastArrayIndexPropertyLimit. That skips the proxy fallback while still passing a 1024-index cap into the factory, so indices beyond that cap can become inaccessible on Hermes.Suggested change
- const bool shouldUseFastArray = - nativeIsMutableArray || - [(NSArray*)nativeObject count] <= kHermesFastArrayIndexPropertyLimit; + const NSUInteger count = [(NSArray*)nativeObject count]; + const bool shouldUseFastArray = + count <= kHermesFastArrayIndexPropertyLimit;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/ObjCBridge.mm` around lines 978 - 997, The fast-array decision wrongly treats nativeIsMutableArray as sufficient to choose the fast path; change the condition that computes shouldUseFastArray to require both that the array is mutable and that its count is <= kHermesFastArrayIndexPropertyLimit (i.e. replace the `||` with `&&` in the expression that sets shouldUseFastArray). This ensures shouldProxyArray is true for large NSMutableArray instances and prevents calling createNativeFastArrayIndexes/factory with a 1024 cap when the array exceeds kHermesFastArrayIndexPropertyLimit.metadata-generator/src/SignatureDispatchEmitter.cpp (2)
53-65:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftPreserve concrete object kinds in the dispatch identity.
dispatchIdcurrently collapsesmdTypeAnyObject/mdTypeNSStringObject/mdTypeNSMutableStringObject/ related object kinds into the same hash, butwrapperKeyand the generated fast argument conversions still depend on the concreteMDTypeKind. That means two ABI-compatible signatures can share onedispatchId, and the firstemplaced wrapper wins for both. ANSString*slot can then execute anidfast-path wrapper, or vice versa.Either keep the concrete object kind in
signatureHash/dispatchId, or make the emitted wrappers fully kind-agnostic. The current mix is not safe.Also applies to: 2335-2407
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metadata-generator/src/SignatureDispatchEmitter.cpp` around lines 53 - 65, The dispatch identity currently collapses concrete object MDTypeKind values which causes distinct concrete kinds (e.g., mdTypeNSStringObject, mdTypeNSMutableStringObject) to map to the same dispatchId while wrappers/fast-paths still depend on the concrete kind; fix this by preserving the concrete MDTypeKind when computing signatureHash/dispatchId instead of canonicalizing them to mdTypeAnyObject — update canonicalizeSignatureTypeKind (or the place that calls it for signature hash creation) to return the original kind for object kinds, ensure dispatchId/signatureHash generation uses that concrete kind, and re-run tests to confirm emplace/select behavior now distinguishes NSString/NSMutableString/id slots (or alternatively make the emitted wrapper conversions fully kind-agnostic if you prefer that approach).
894-926:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject lossy 64-bit
BigIntfast conversions (uselosslessto fail/fallback).The code passes a
losslessout-parameter tonapi_get_value_bigint_int64/napi_get_value_bigint_uint64(andv8::BigInt::Int64Value/Uint64Value) but never checks it. Whenlossless == false, the APIs return a truncated/wrapped 64-bit value, so oversizedBigInts can silently corrupt the native value on these fast paths. (N-API: ~899/914; V8: ~1061/1076, also covering ~1056-1088)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metadata-generator/src/SignatureDispatchEmitter.cpp` around lines 894 - 926, The BigInt-fast-paths call napi_get_value_bigint_int64 / napi_get_value_bigint_uint64 but ignore the out-parameter lossless{index}, allowing truncated values; update the branches that call napi_get_value_bigint_int64 and napi_get_value_bigint_uint64 to check lossless{index} after a successful return and treat lossless==false as an error: if lossless{index} is false then call cleanupManagedArgs() when hasCleanupArgs is true and return false to force the fallback; ensure the same check is applied for both signed (napi_get_value_bigint_int64) and unsigned (napi_get_value_bigint_uint64) paths referencing arg{index}, lossless{index}, and cleanupManagedArgs().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/ffi/CFunction.mm`:
- Around line 436-451: The code eagerly allocates a
shared_ptr<CFunctionInvocationFrame> (invocationFrame) and its heap per-argument
storage even on the fast direct-invoker path; move to lazy allocation by keeping
a cheap inline/stack return buffer (rvalue or small local storage) and only
construct CFunctionInvocationFrame (and call make_shared) when falling back to
the slow path (when engineDirectInvoker==nullptr && (napiInvoker==nullptr ||
cif->skipGeneratedNapiDispatch) OR when preparedInvoker/ffi_call or
isMainEntrypoint scheduling is required). Update the call site in the function
that currently creates invocationFrame (the jsCallDirect / CFunction invocation
logic) to use the inline rvalue for direct dispatch and only allocate
invocationFrame before calling preparedInvoker/ffi_call or before entering the
try block when the path requires full frame semantics.
---
Outside diff comments:
In `@metadata-generator/src/SignatureDispatchEmitter.cpp`:
- Around line 53-65: The dispatch identity currently collapses concrete object
MDTypeKind values which causes distinct concrete kinds (e.g.,
mdTypeNSStringObject, mdTypeNSMutableStringObject) to map to the same dispatchId
while wrappers/fast-paths still depend on the concrete kind; fix this by
preserving the concrete MDTypeKind when computing signatureHash/dispatchId
instead of canonicalizing them to mdTypeAnyObject — update
canonicalizeSignatureTypeKind (or the place that calls it for signature hash
creation) to return the original kind for object kinds, ensure
dispatchId/signatureHash generation uses that concrete kind, and re-run tests to
confirm emplace/select behavior now distinguishes NSString/NSMutableString/id
slots (or alternatively make the emitted wrapper conversions fully kind-agnostic
if you prefer that approach).
- Around line 894-926: The BigInt-fast-paths call napi_get_value_bigint_int64 /
napi_get_value_bigint_uint64 but ignore the out-parameter lossless{index},
allowing truncated values; update the branches that call
napi_get_value_bigint_int64 and napi_get_value_bigint_uint64 to check
lossless{index} after a successful return and treat lossless==false as an error:
if lossless{index} is false then call cleanupManagedArgs() when hasCleanupArgs
is true and return false to force the fallback; ensure the same check is applied
for both signed (napi_get_value_bigint_int64) and unsigned
(napi_get_value_bigint_uint64) paths referencing arg{index}, lossless{index},
and cleanupManagedArgs().
In `@NativeScript/ffi/ObjCBridge.mm`:
- Around line 978-997: The fast-array decision wrongly treats
nativeIsMutableArray as sufficient to choose the fast path; change the condition
that computes shouldUseFastArray to require both that the array is mutable and
that its count is <= kHermesFastArrayIndexPropertyLimit (i.e. replace the `||`
with `&&` in the expression that sets shouldUseFastArray). This ensures
shouldProxyArray is true for large NSMutableArray instances and prevents calling
createNativeFastArrayIndexes/factory with a 1024 cap when the array exceeds
kHermesFastArrayIndexPropertyLimit.
In `@NativeScript/ffi/TypeConv.mm`:
- Around line 2823-2839: The toJS override currently returns nullptr when
ObjCBridgeState::InstanceData(env) is missing, which yields an invalid
napi_value; instead, keep the pointer fallback for non-null Class values: in
toJS (the function containing Class cls, the findRegisteredClassConstructor
call, and the ObjCBridgeState::InstanceData(env) lookup), if bridgeState is null
call the same pointer-to-JS fallback used elsewhere in this file (i.e.,
construct a valid napi_value representing the raw Class pointer or external
handle) and return that rather than nullptr so callers receive a valid JS handle
when getObject cannot be used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53cd0205-eb24-422f-b986-c814681c5f86
📒 Files selected for processing (18)
.github/workflows/npm_trusted_release.ymlNativeScript/ffi/CFunction.mmNativeScript/ffi/Class.mmNativeScript/ffi/EngineDirectCall.mmNativeScript/ffi/HermesFastCallbackInfo.hNativeScript/ffi/HermesFastNativeApi.mmNativeScript/ffi/JSCFastNativeApi.hNativeScript/ffi/JSCFastNativeApi.mmNativeScript/ffi/ObjCBridge.hNativeScript/ffi/ObjCBridge.mmNativeScript/ffi/QuickJSFastNativeApi.mmNativeScript/ffi/SignatureDispatch.hNativeScript/ffi/TypeConv.mmNativeScript/ffi/jsi/NativeApiJsiReactNative.hbenchmarks/objc-dispatch/run.jsexamples/react-native-demo/App.tsxmetadata-generator/src/SignatureDispatchEmitter.cpppackages/react-native/ios/Fabric/NativeScriptUIViewComponentView.mm
💤 Files with no reviewable changes (1)
- packages/react-native/ios/Fabric/NativeScriptUIViewComponentView.mm
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NativeScript/ffi/QuickJSFastNativeApi.mm (1)
1567-1596:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDecode
mdTypeUShortstrings as characters, not UTF-8 bytes.This path treats a JS string as a one-byte UTF-8 buffer. A single non-ASCII character like
éor中will either be rejected (length != 1) or decoded incorrectly, while the JSC path reads a single JS character/code unit. That makes the QuickJS fast path observably inconsistent for validunichar/unsigned shortinputs.Suggested fix
bool TryFastConvertQuickJSUInt16Argument(napi_env env, napi_value value, uint16_t* result) { if (env == nullptr || value == nullptr || result == nullptr) { return false; } JSValue jsValue = ToJSValue(value); if (JS_IsString(jsValue)) { - size_t length = 0; - const char* str = JS_ToCStringLen(env->context, &length, jsValue); + size_t byteLength = 0; + const char* str = JS_ToCStringLen(env->context, &byteLength, jsValue); if (str == nullptr) { return false; } - if (length != 1) { + + NSString* string = + [[[NSString alloc] initWithBytes:str + length:byteLength + encoding:NSUTF8StringEncoding] autorelease]; + JS_FreeCString(env->context, str); + + if (string == nil || [string length] != 1) { - JS_FreeCString(env->context, str); napi_throw_type_error(env, nullptr, "Expected a single-character string."); return false; } - *result = static_cast<uint8_t>(str[0]); - JS_FreeCString(env->context, str); + + *result = static_cast<uint16_t>([string characterAtIndex:0]); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/QuickJSFastNativeApi.mm` around lines 1567 - 1596, The fast-path string branch in TryFastConvertQuickJSUInt16Argument wrongly treats the JS string as a UTF-8 byte buffer; instead convert the JSValue to a QuickJS JSString and read the first UTF-16 code unit (i.e. first JS character/code unit), not the first UTF-8 byte. Replace the JS_ToCStringLen/length check with a JS_ToString (or JS_DupValue/JS_VALUE_GET_STRING) + JS_GetStringLen to ensure there is exactly one code unit, then extract that code unit (via the QuickJS string char accessor such as JS_GetStringChar/appropriate API) and store it into *result; keep the existing error handling (napi_throw_type_error) when it isn’t a single code unit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/ffi/CFunction.mm`:
- Around line 517-523: The CFunctionInvocationFrame currently allocates rvalue_
using cif->rvalueLength > 0 ? cif->rvalueLength : 1 which can under-allocate
when cif->rvalueLength is zero; update the allocation logic in
CFunctionInvocationFrame (the rvalue_ allocation and any related sizing logic
used before calling preparedInvoker/ffi_call) to mirror CFunctionReturnStorage's
behavior by using cif->rvalueLength if > 0 otherwise falling back to
cif->cif.rtype->size so the return buffer is correctly sized for the slow path.
---
Outside diff comments:
In `@NativeScript/ffi/QuickJSFastNativeApi.mm`:
- Around line 1567-1596: The fast-path string branch in
TryFastConvertQuickJSUInt16Argument wrongly treats the JS string as a UTF-8 byte
buffer; instead convert the JSValue to a QuickJS JSString and read the first
UTF-16 code unit (i.e. first JS character/code unit), not the first UTF-8 byte.
Replace the JS_ToCStringLen/length check with a JS_ToString (or
JS_DupValue/JS_VALUE_GET_STRING) + JS_GetStringLen to ensure there is exactly
one code unit, then extract that code unit (via the QuickJS string char accessor
such as JS_GetStringChar/appropriate API) and store it into *result; keep the
existing error handling (napi_throw_type_error) when it isn’t a single code
unit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84bb766d-6eab-4ddf-9ed8-1f1affedf84c
📒 Files selected for processing (3)
NativeScript/ffi/CFunction.mmNativeScript/ffi/JSCFastNativeApi.mmNativeScript/ffi/QuickJSFastNativeApi.mm
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Generated by Codex. Current head: This pass was focused on making the PR human-reviewable again after the direct-engine FFI work grew sideways. Suggested review path:
Verification on this final head:
CodeRabbit state: latest explicit review command returned without new actionable comments, and thread-aware GraphQL check shows |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
NativeScript/ffi/napi/ClassMember.mm (3)
2050-2051:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse separate free flags for the setter conversion.
Passing
&shouldFreefor both out-parameters aliases the per-argument flag with the aggregate flag. IftoNativeupdates the aggregate flag independently, this path can free memory it does not own. Keep the same two-variable pattern used in the other call sites.Suggested fix
- bool shouldFree = false; - cif->argTypes[0]->toNative(env, value, avalues[2], &shouldFree, &shouldFree); + bool shouldFree = false; + bool shouldFreeAny = false; + cif->argTypes[0]->toNative(env, value, avalues[2], &shouldFree, &shouldFreeAny);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/napi/ClassMember.mm` around lines 2050 - 2051, The setter conversion uses a single bool shouldFree for both per-argument and aggregate out-parameters, which can cause incorrect frees; change this to two separate flags (e.g., bool argShouldFree = false; bool aggShouldFree = false) and pass them respectively to cif->argTypes[0]->toNative(env, value, avalues[2], &argShouldFree, &aggShouldFree) so the per-argument free state is not aliased with the aggregate flag used elsewhere (locate the call site that currently uses shouldFree and update it to the two-variable pattern).
385-391:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet a failed generated ObjC dispatch fall back to
objcNativeCall.A
falseresult from the generated invoker currently bubbles up as a hard failure to every caller. For fast-path conversion misses, that should fall through to the existing libffi path instead of aborting the call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/napi/ClassMember.mm` around lines 385 - 391, The generated ObjC dispatch currently returns false on a failed invoker; instead, when invoked == false you should call the libffi fallback objcNativeCall so the fast-path miss falls through. Modify the block in ClassMember.mm where engineInvoker/invoker are called: if (!invoked) invoke objcNativeCall(env, cif, (void*)objc_msgSend, self, selector, argv, rvalue) and return its result rather than returning false. Keep existing scope handling (NativeCallRuntimeUnlockScope) and use the same parameters as the original call so callers receive the fallback behavior.
582-599:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat arrays as compatible with
NSStringparameters during scoring.This branch makes any array look valid for
mdTypeNSStringObject/mdTypeNSMutableStringObject, so overload and initializer resolution can prefer a string-taking signature for array input before conversion ever runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/napi/ClassMember.mm` around lines 582 - 599, The code incorrectly marks JS arrays as compatible with mdTypeNSStringObject/mdTypeNSMutableStringObject during type scoring; change the napi_is_array branch so arrays are not treated as valid NSString candidates (do not return true for isArray). In the switch handling mdTypeNSStringObject/mdTypeNSMutableStringObject (the block referencing jsType, napi_is_array, napi_unwrap), if napi_is_array reports true, skip returning true and proceed to the wrapped-object check (only return true when napi_unwrap succeeds); ensure no early true is returned for arrays so scoring won't prefer string-taking overloads for array inputs.NativeScript/ffi/napi/CFunction.mm (1)
493-501:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the libffi fallback when the generated C-function path declines.
If
engineDirectInvoker/napiInvokerreturnsfalse, this returnsnullptrimmediately and never reaches the slow path below. That changes behavior for signatures the generated path can't handle even though the existing libffi path still can.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/napi/CFunction.mm` around lines 493 - 501, The current early return when engineDirectInvoker or napiInvoker returns false prevents falling back to the existing libffi slow path; instead of returning nullptr when invoked is false, remove the early return and allow execution to continue to the slow/libffi path so signatures the generated C-function path can't handle still work; update the block around the invoked check in CFunction.mm (where returnStorage.rvalue(), engineDirectInvoker, napiInvoker and the invoked boolean are used) to skip the early return and proceed to the code that performs the libffi fallback.NativeScript/ffi/hermes/HermesFastConversion.mm (1)
1017-1025:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject lossily converted BigInt pointer values in the fast path.
In
NativeScript/ffi/hermes/HermesFastConversion.mm(TryFastConvertHermesPointerArgument,case napi_bigint), the code ignores thelosslessout parameter fromnapi_get_value_bigint_uint64. Iflossless == false, the returneduint64_tmay be truncated and is currently reinterpreted as a pointer, risking an incorrect address instead of falling back.Suggested fix
case napi_bigint: { uint64_t raw = 0; bool lossless = false; if (napi_get_value_bigint_uint64(env, value, &raw, &lossless) != - napi_ok) { + napi_ok || !lossless) { return false; } *result = reinterpret_cast<void*>(raw); return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/hermes/HermesFastConversion.mm` around lines 1017 - 1025, In TryFastConvertHermesPointerArgument (case napi_bigint) handle the napi_get_value_bigint_uint64 lossless out parameter: after calling napi_get_value_bigint_uint64, verify the bool lossless is true and if not return false so the fast path rejects lossy BigInt conversions; only when napi_get_value_bigint_uint64 succeeds and lossless == true assign *result = reinterpret_cast<void*>(raw) and return true.NativeScript/ffi/jsc/JSCFastConversion.mm (1)
707-709:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire checking
losslessfor BigInt→64-bit JSC fast conversions
NativeScript/ffi/jsc/JSCFastConversion.mmcallsnapi_get_value_bigint_int64/napi_get_value_bigint_uint64with alosslessout-parameter, but the code returns success solely based on== napi_ok, ignoringlossless. For out-of-range BigInt inputs,losslesswill befalsewhilenapi_okcan still be returned—so these paths silently truncate and still accept the value. Fix by treating!losslessas conversion failure (or falling back to a non-truncating path).Affected ranges: 707-709, 741-743, 954-962.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/jsc/JSCFastConversion.mm` around lines 707 - 709, The current BigInt→64-bit conversion calls (napi_get_value_bigint_int64 and napi_get_value_bigint_uint64) ignore the lossless out-parameter and only check for napi_ok; update each call (the occurrences around the earlier return, and the blocks at the other two ranges) to treat lossless==false as a conversion failure: after calling napi_get_value_bigint_int64/uint64, return false (or take the non‑truncating fallback) if the call succeeded but lossless is false. Concretely, keep the existing napi_ok check but also ensure the function returns false when lossless is false (or proceeds to the safe fallback path) for the calls to napi_get_value_bigint_int64 and napi_get_value_bigint_uint64 using the lossless variable and the result out-parameter.NativeScript/ffi/shared/EngineDirectCall.mm (2)
1000-1002:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
envbefore reading bridge state.Line 1000 calls
ObjCBridgeState::InstanceData(env)before the null check, so a nullenvnow dereferences through the bridge lookup instead of bailing out cleanly.Suggested fix
- ObjCBridgeState* bridgeState = ObjCBridgeState::InstanceData(env); - if (env == nullptr || bridgeState == nullptr) { + if (env == nullptr) { + return nullptr; + } + ObjCBridgeState* bridgeState = ObjCBridgeState::InstanceData(env); + if (bridgeState == nullptr) { return nullptr; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/shared/EngineDirectCall.mm` around lines 1000 - 1002, The code currently calls ObjCBridgeState::InstanceData(env) before verifying env is non-null; change the order to first guard against a null env (e.g., if (env == nullptr) return nullptr;) and only then call ObjCBridgeState::InstanceData(env), then check the returned bridgeState for null before proceeding; update the block around ObjCBridgeState::InstanceData and the existing env/bridgeState null checks to reflect this reordering.
793-815:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't reuse
cif->avaluesas per-call scratch storage.
function->cifis cached and shared, so writing converted arguments intocif->avalues[i]makes concurrent or reentrant calls race on the same buffers. One invocation can overwrite or free another invocation's arguments beforeffi_callruns. The ObjC path already avoids this by allocating per-invocation storage.Suggested fix
void* stackAvalues[16]; std::vector<void*> heapAvalues; void** avalues = stackAvalues; if (cif->argc > 16) { heapAvalues.resize(cif->argc); avalues = heapAvalues.data(); } + + EngineDirectArgumentStorage argStorage(cif, 0); + if (!argStorage.valid()) { + napi_throw_error(env, "NativeScriptException", + "Unable to allocate argument storage for C function call."); + return false; + } bool stackShouldFree[16] = {}; std::vector<uint8_t> heapShouldFree; if (cif->argc > 16) { heapShouldFree.reserve(cif->argc); @@ for (unsigned int i = 0; i < cif->argc; i++) { bool shouldFreeArg = false; - avalues[i] = cif->avalues[i]; + avalues[i] = argStorage.at(i); if (!TryFastConvertEngineArgument(env, cif->argTypes[i]->kind, invocationArgs[i], avalues[i])) { cif->argTypes[i]->toNative(env, invocationArgs[i], avalues[i], &shouldFreeArg, &shouldFreeAny); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/shared/EngineDirectCall.mm` around lines 793 - 815, The current loop writes converted args into the shared cached buffer cif->avalues, causing races; allocate and use per-invocation storage (e.g., use stackAvalues/heapAvalues via the local pointer avalues) and never write back into cif->avalues; copy cif->avalues into avalues initially, run TryFastConvertEngineArgument and call cif->argTypes[i]->toNative to place results into avalues[i], and maintain per-call shouldFree storage (stackShouldFree/heapShouldFree via a similar pointer) and update shouldFreeAny accordingly so frees happen only for this invocation; keep references to TryFastConvertEngineArgument, cif->avalues, avalues, stackAvalues, heapAvalues, stackShouldFree, heapShouldFree, and cif->argTypes[i]->toNative to locate and apply the change.NativeScript/ffi/napi/TypeConv.mm (1)
1321-1334:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the raw-external
__ns_native_ptrfallback here.
resolveCachedHandleObject()still accepts__ns_native_ptras either aPointerinstance or a raw external, but this new fast path only unwraps thePointershape. Wrappers that still stash an external in__ns_native_ptrwill now fall through as invalid native handles even though the slow path still supports them.Suggested fix
if (napi_has_named_property(env, input, "__ns_native_ptr", &hasNativePointer) == napi_ok && hasNativePointer) { napi_value nativePointerValue = nullptr; - if (napi_get_named_property(env, input, "__ns_native_ptr", &nativePointerValue) == - napi_ok && - Pointer::isInstance(env, nativePointerValue)) { - Pointer* pointer = Pointer::unwrap(env, nativePointerValue); - if (pointer != nullptr && pointer->data != nullptr) { - *out = pointer->data; - return true; - } + if (napi_get_named_property(env, input, "__ns_native_ptr", &nativePointerValue) == + napi_ok) { + if (Pointer::isInstance(env, nativePointerValue)) { + Pointer* pointer = Pointer::unwrap(env, nativePointerValue); + if (pointer != nullptr && pointer->data != nullptr) { + *out = pointer->data; + return true; + } + } else { + void* nativePointer = nullptr; + if (napi_get_value_external(env, nativePointerValue, &nativePointer) == napi_ok && + nativePointer != nullptr) { + *out = nativePointer; + return true; + } + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/napi/TypeConv.mm` around lines 1321 - 1334, The fast-path currently only handles Pointer-shaped objects and ignores raw externals stored under "__ns_native_ptr", so restore the fallback: after retrieving nativePointerValue (using napi_get_named_property) and if Pointer::isInstance(env, nativePointerValue) check unwrap as before (Pointer::unwrap) but otherwise detect a raw external (use napi_get_value_external / napi_typeof or equivalent N-API check) and extract the void* from that external into *out and return true; keep existing napi_has_named_property / napi_get_named_property and the Pointer::isInstance / Pointer::unwrap flow intact so both Pointer instances and raw externals are supported.NativeScript/ffi/shared/SignatureDispatch.h (3)
420-439:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
int8_t/uint8_treturns numeric, not boolean.
SetHermesGeneratedInt8ReturnandSetHermesGeneratedUInt8Returncurrently encode0/1with the Hermes bool tag. That changes the JS type for numeric signatures: anint8_treturn of1comes back astrueinstead of1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/shared/SignatureDispatch.h` around lines 420 - 439, The functions SetHermesGeneratedInt8Return and SetHermesGeneratedUInt8Return are special-casing 0/1 to produce Hermes boolean-tagged values, which makes numeric signatures return booleans; change both to always produce numeric returns: remove the if (value == 0 || value == 1) branches and call makeHermesDispatchRawNumberValue(cif, static_cast<double>(value)) unconditionally for both int8_t and uint8_t, leaving the function signatures and return true; this ensures int8_t/uint8_t results are always numeric in JS.
250-407:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject out-of-range Hermes numeric arguments before narrowing.
These fast-path helpers only check
isfinite()and thenstatic_castinto integral types. Large or fractional JS numbers can wrap, truncate, or hit undefined narrowing behavior here instead of falling back to the regular converter, so generated Hermes dispatch can pass corrupted native arguments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/shared/SignatureDispatch.h` around lines 250 - 407, Fast-path Hermes numeric converters (e.g., TryFastConvertHermesGeneratedInt8Argument, TryFastConvertHermesGeneratedUInt32RawArgument, TryFastConvertHermesGeneratedInt64RawArgument, etc.) only check finiteness and then static_cast, which allows out-of-range or fractional JS numbers to silently wrap/truncate; update each TryFastConvertHermesGenerated*Argument and *RawArgument to validate the converted double before narrowing by (1) ensuring the value is within the target type range using std::numeric_limits<T>::min()/max(), (2) for integer types rejecting fractional values (e.g., via std::trunc/ std::floor or checking converted == std::trunc(converted)), and (3) for unsigned types also enforcing >= 0; if any check fails, return false so the caller falls back to the regular converter; apply the same validation to raw variants after calling readHermesDispatchFiniteNumberRaw.
832-839:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
NS_DISABLE_GSD=1still leaves generated dispatch enabled.The current predicate only disables GSD when the variable is exactly
"0". In practice the common kill-switch value is"1", so this makesNS_DISABLE_GSDineffective for the normal disable case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/shared/SignatureDispatch.h` around lines 832 - 839, The isGeneratedDispatchEnabled() predicate treats only the exact string "0" as disabling GSD, so NS_DISABLE_GSD=1 still enables it; update the function (isGeneratedDispatchEnabled) to parse common truthy disable values instead: if NS_DISABLE_GSD is unset or empty return true, but if it is set to "1" or any case-insensitive "true" (or other common truthy indicators) return false; implement this by normalizing the value from std::getenv("NS_DISABLE_GSD") and checking for '1' or a case-insensitive match to "true" (and keep the existing check for exact "0" as disabling if you prefer) so that setting NS_DISABLE_GSD=1 properly disables generated dispatch.NativeScript/napi/quickjs/quickjs-api.c (1)
3414-3425:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't cache the wrap pointer outside its owning property.
The new
JS_SetOpaque(jsValue, externalInfo)fast path makes the object-level opaque slot point at storage that is still owned by the writable/configurablenapi_externalproperty. If that property is deleted or replaced, the external wrapper can be finalized while the opaque slot still points at the oldExternalInfo, and the newnapi_unwrap()path will read freed memory. Either make the backing property immutable/truly hidden, or revalidate the property before trusting the cached opaque slot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/napi/quickjs/quickjs-api.c` around lines 3414 - 3425, The code caches externalInfo into the object opaque slot via JS_SetOpaque(jsValue, externalInfo) after defining the napi_external property (JS_DefinePropertyValue), but that property is writable/configurable so it can be deleted/replaced and free externalInfo while the opaque slot still points to it; fix by removing this unsafe fast path or making the backing property truly immutable/hidden before caching (e.g., define napi_external as non-writable, non-configurable, and non-enumerable) or, if you must keep a writable property, avoid trusting the opaque cache and instead revalidate the napi_external property (read and compare its current value) each time before returning the cached pointer (symbols: JS_DefinePropertyValue, napi_external, JS_SetOpaque, externalInfo, napi_unwrap).
♻️ Duplicate comments (1)
NativeScript/ffi/v8/V8FastConversion.mm (1)
360-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't coerce non-finite V8 numbers to
0.0.Normalizing
NaN/±Infinityto zero silently changes the argument on the generated path instead of letting the regular conversion/error path handle it consistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/ffi/v8/V8FastConversion.mm` around lines 360 - 381, The generated-path conversion handlers mdTypeFloat and mdTypeDouble currently coerce NaN/±Inf to 0.0; remove the std::isnan/std::isinf checks and the converted = 0.0 assignments so the code simply calls value->NumberValue(env->context()).To(&converted) and then writes converted (cast for float) into result, allowing NaN/Infinity to propagate the same way as the regular conversion/error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/ffi/hermes/HermesFastNativeApi.mm`:
- Around line 707-745: The code marks the Hermes fast path handled (writes to
*handled) before the final dispatch, causing a hard failure on a fast-path miss;
change the logic so *handled is only set after the dispatch succeeds (i.e.,
after didInvoke is true). Specifically, in the block that uses
EngineDirectRoundTripCacheFrameGuard, invoker
(ensureHermesObjCEngineDirectInvoker) and the fallback
InvokeObjCMemberEngineDirectDynamic, move the "*handled = true" assignment so it
executes only when didInvoke is true (after the `@try/`@catch and the if
(!didInvoke) check), and apply the same change to the other analogous block that
uses the same symbols (invoker, didInvoke, InvokeObjCMemberEngineDirectDynamic)
later in the file.
In `@NativeScript/ffi/hermes/HermesFastNativeApiPrivate.h`:
- Around line 78-89: The current makeHermesRawValue writes into
cif->hermesRawReturnSlot which can be clobbered by reentrant calls; instead, do
not store raw bits in the shared Cif. Change makeHermesRawValue so it never
writes to cif->hermesRawReturnSlot: always allocate a per-call/thread-local slot
(like the existing static thread_local slots/nextSlot) and return a pointer to
that slot; keep the same slot rotation logic and reinterpret_cast to napi_value,
and remove the branch that assigns to cif->hermesRawReturnSlot so Cif is not
used to hold return bits.
In `@NativeScript/ffi/jsc/JSCFastNativeApi.mm`:
- Around line 47-57: The fallback that returns member->cls->nativeClass when
member->classMethod or when jsThis is a function is too broad and causes
inherited constructor calls to resolve to the superclass; update the logic in
the block checking member, member->cls, and member->cls->nativeClass to also
perform the same constructor-identity check used in the N-API/Hermes path: when
jsThis is a function (napi_typeof(...)=napi_function) confirm that jsThis's JS
constructor identity equals member->cls->nativeClass before returning
static_cast<id>(member->cls->nativeClass); if the identity does not match, do
not fall back to member->cls->nativeClass and allow dispatch to use the actual
jsThis/receiver instead.
In `@NativeScript/ffi/quickjs/QuickJSFastNativeApi.mm`:
- Around line 36-42: The fast-path in QuickJSFastNativeApi.mm returns the raw
wrapped pointer from TryUnwrapQuickJSNativeObjectFast (called with env,
ToJSValue(jsThis)), which may be an ObjCClass/ObjCProtocol wrapper rather than
the real Objective‑C receiver; update the code so that after obtaining non-null
wrapped you perform the same class/protocol normalization used by the slow
conversion path (i.e. transform bridge wrappers into the actual Class/id
receiver) before returning it — reuse the existing normalization helper used
elsewhere (the same logic the slower conversion path uses) so objc_msgSend
always receives the normalized native receiver.
In `@NativeScript/ffi/v8/V8FastConversion.mm`:
- Around line 270-276: In TryFastConvertV8Argument's string branch (the
value->IsString() block using v8::String::Value chars), remove the call to
throwV8Error so the fast-path probe does not create a pending V8 exception;
instead, on chars.length() != 1 set *result = 0 and return false to signal the
caller to run the generic converter. Do not introduce any other throws or V8
exception APIs in this probe so callers can safely interpret false as “fallback
only.”
- Around line 337-350: The fast-path currently unconditionally narrows BigInt
via Int64Value()/Uint64Value(), which can truncate; modify the BigInt handling
in V8FastConversion.mm so that after calling
value.As<v8::BigInt>()->Int64Value(&lossless) or ->Uint64Value(&lossless) you
check the lossless flag and only write to *reinterpret_cast<int64_t*>(result) or
*reinterpret_cast<uint64_t*>(result) and return true if lossless is true; if
lossless is false, do not assign and return false (so the caller falls back to
the slower, safe conversion path) — apply this change for both the signed
(Int64Value) and unsigned (Uint64Value) branches.
In `@NativeScript/ffi/v8/V8FastNativeWrapper.mm`:
- Around line 87-89: The named-setter path calls definePlainValueProperty (which
calls CreateDataProperty) but always returns NS_V8_RETURN_YES; change it to
check the boolean result of definePlainValueProperty (or the underlying
CreateDataProperty) when setting holder/property/value and if it returns false
return NS_V8_RETURN_NO instead of unconditionally NS_V8_RETURN_YES so failed
writes (e.g., on non-extensible/frozen wrappers) propagate failure to JS; update
the code around definePlainValueProperty(..., holder, property, value) to
capture its return and conditionally use NS_V8_RETURN_YES or NS_V8_RETURN_NO.
---
Outside diff comments:
In `@NativeScript/ffi/hermes/HermesFastConversion.mm`:
- Around line 1017-1025: In TryFastConvertHermesPointerArgument (case
napi_bigint) handle the napi_get_value_bigint_uint64 lossless out parameter:
after calling napi_get_value_bigint_uint64, verify the bool lossless is true and
if not return false so the fast path rejects lossy BigInt conversions; only when
napi_get_value_bigint_uint64 succeeds and lossless == true assign *result =
reinterpret_cast<void*>(raw) and return true.
In `@NativeScript/ffi/jsc/JSCFastConversion.mm`:
- Around line 707-709: The current BigInt→64-bit conversion calls
(napi_get_value_bigint_int64 and napi_get_value_bigint_uint64) ignore the
lossless out-parameter and only check for napi_ok; update each call (the
occurrences around the earlier return, and the blocks at the other two ranges)
to treat lossless==false as a conversion failure: after calling
napi_get_value_bigint_int64/uint64, return false (or take the non‑truncating
fallback) if the call succeeded but lossless is false. Concretely, keep the
existing napi_ok check but also ensure the function returns false when lossless
is false (or proceeds to the safe fallback path) for the calls to
napi_get_value_bigint_int64 and napi_get_value_bigint_uint64 using the lossless
variable and the result out-parameter.
In `@NativeScript/ffi/napi/CFunction.mm`:
- Around line 493-501: The current early return when engineDirectInvoker or
napiInvoker returns false prevents falling back to the existing libffi slow
path; instead of returning nullptr when invoked is false, remove the early
return and allow execution to continue to the slow/libffi path so signatures the
generated C-function path can't handle still work; update the block around the
invoked check in CFunction.mm (where returnStorage.rvalue(),
engineDirectInvoker, napiInvoker and the invoked boolean are used) to skip the
early return and proceed to the code that performs the libffi fallback.
In `@NativeScript/ffi/napi/ClassMember.mm`:
- Around line 2050-2051: The setter conversion uses a single bool shouldFree for
both per-argument and aggregate out-parameters, which can cause incorrect frees;
change this to two separate flags (e.g., bool argShouldFree = false; bool
aggShouldFree = false) and pass them respectively to
cif->argTypes[0]->toNative(env, value, avalues[2], &argShouldFree,
&aggShouldFree) so the per-argument free state is not aliased with the aggregate
flag used elsewhere (locate the call site that currently uses shouldFree and
update it to the two-variable pattern).
- Around line 385-391: The generated ObjC dispatch currently returns false on a
failed invoker; instead, when invoked == false you should call the libffi
fallback objcNativeCall so the fast-path miss falls through. Modify the block in
ClassMember.mm where engineInvoker/invoker are called: if (!invoked) invoke
objcNativeCall(env, cif, (void*)objc_msgSend, self, selector, argv, rvalue) and
return its result rather than returning false. Keep existing scope handling
(NativeCallRuntimeUnlockScope) and use the same parameters as the original call
so callers receive the fallback behavior.
- Around line 582-599: The code incorrectly marks JS arrays as compatible with
mdTypeNSStringObject/mdTypeNSMutableStringObject during type scoring; change the
napi_is_array branch so arrays are not treated as valid NSString candidates (do
not return true for isArray). In the switch handling
mdTypeNSStringObject/mdTypeNSMutableStringObject (the block referencing jsType,
napi_is_array, napi_unwrap), if napi_is_array reports true, skip returning true
and proceed to the wrapped-object check (only return true when napi_unwrap
succeeds); ensure no early true is returned for arrays so scoring won't prefer
string-taking overloads for array inputs.
In `@NativeScript/ffi/napi/TypeConv.mm`:
- Around line 1321-1334: The fast-path currently only handles Pointer-shaped
objects and ignores raw externals stored under "__ns_native_ptr", so restore the
fallback: after retrieving nativePointerValue (using napi_get_named_property)
and if Pointer::isInstance(env, nativePointerValue) check unwrap as before
(Pointer::unwrap) but otherwise detect a raw external (use
napi_get_value_external / napi_typeof or equivalent N-API check) and extract the
void* from that external into *out and return true; keep existing
napi_has_named_property / napi_get_named_property and the Pointer::isInstance /
Pointer::unwrap flow intact so both Pointer instances and raw externals are
supported.
In `@NativeScript/ffi/shared/EngineDirectCall.mm`:
- Around line 1000-1002: The code currently calls
ObjCBridgeState::InstanceData(env) before verifying env is non-null; change the
order to first guard against a null env (e.g., if (env == nullptr) return
nullptr;) and only then call ObjCBridgeState::InstanceData(env), then check the
returned bridgeState for null before proceeding; update the block around
ObjCBridgeState::InstanceData and the existing env/bridgeState null checks to
reflect this reordering.
- Around line 793-815: The current loop writes converted args into the shared
cached buffer cif->avalues, causing races; allocate and use per-invocation
storage (e.g., use stackAvalues/heapAvalues via the local pointer avalues) and
never write back into cif->avalues; copy cif->avalues into avalues initially,
run TryFastConvertEngineArgument and call cif->argTypes[i]->toNative to place
results into avalues[i], and maintain per-call shouldFree storage
(stackShouldFree/heapShouldFree via a similar pointer) and update shouldFreeAny
accordingly so frees happen only for this invocation; keep references to
TryFastConvertEngineArgument, cif->avalues, avalues, stackAvalues, heapAvalues,
stackShouldFree, heapShouldFree, and cif->argTypes[i]->toNative to locate and
apply the change.
In `@NativeScript/ffi/shared/SignatureDispatch.h`:
- Around line 420-439: The functions SetHermesGeneratedInt8Return and
SetHermesGeneratedUInt8Return are special-casing 0/1 to produce Hermes
boolean-tagged values, which makes numeric signatures return booleans; change
both to always produce numeric returns: remove the if (value == 0 || value == 1)
branches and call makeHermesDispatchRawNumberValue(cif,
static_cast<double>(value)) unconditionally for both int8_t and uint8_t, leaving
the function signatures and return true; this ensures int8_t/uint8_t results are
always numeric in JS.
- Around line 250-407: Fast-path Hermes numeric converters (e.g.,
TryFastConvertHermesGeneratedInt8Argument,
TryFastConvertHermesGeneratedUInt32RawArgument,
TryFastConvertHermesGeneratedInt64RawArgument, etc.) only check finiteness and
then static_cast, which allows out-of-range or fractional JS numbers to silently
wrap/truncate; update each TryFastConvertHermesGenerated*Argument and
*RawArgument to validate the converted double before narrowing by (1) ensuring
the value is within the target type range using
std::numeric_limits<T>::min()/max(), (2) for integer types rejecting fractional
values (e.g., via std::trunc/ std::floor or checking converted ==
std::trunc(converted)), and (3) for unsigned types also enforcing >= 0; if any
check fails, return false so the caller falls back to the regular converter;
apply the same validation to raw variants after calling
readHermesDispatchFiniteNumberRaw.
- Around line 832-839: The isGeneratedDispatchEnabled() predicate treats only
the exact string "0" as disabling GSD, so NS_DISABLE_GSD=1 still enables it;
update the function (isGeneratedDispatchEnabled) to parse common truthy disable
values instead: if NS_DISABLE_GSD is unset or empty return true, but if it is
set to "1" or any case-insensitive "true" (or other common truthy indicators)
return false; implement this by normalizing the value from
std::getenv("NS_DISABLE_GSD") and checking for '1' or a case-insensitive match
to "true" (and keep the existing check for exact "0" as disabling if you prefer)
so that setting NS_DISABLE_GSD=1 properly disables generated dispatch.
In `@NativeScript/napi/quickjs/quickjs-api.c`:
- Around line 3414-3425: The code caches externalInfo into the object opaque
slot via JS_SetOpaque(jsValue, externalInfo) after defining the napi_external
property (JS_DefinePropertyValue), but that property is writable/configurable so
it can be deleted/replaced and free externalInfo while the opaque slot still
points to it; fix by removing this unsafe fast path or making the backing
property truly immutable/hidden before caching (e.g., define napi_external as
non-writable, non-configurable, and non-enumerable) or, if you must keep a
writable property, avoid trusting the opaque cache and instead revalidate the
napi_external property (read and compare its current value) each time before
returning the cached pointer (symbols: JS_DefinePropertyValue, napi_external,
JS_SetOpaque, externalInfo, napi_unwrap).
---
Duplicate comments:
In `@NativeScript/ffi/v8/V8FastConversion.mm`:
- Around line 360-381: The generated-path conversion handlers mdTypeFloat and
mdTypeDouble currently coerce NaN/±Inf to 0.0; remove the std::isnan/std::isinf
checks and the converted = 0.0 assignments so the code simply calls
value->NumberValue(env->context()).To(&converted) and then writes converted
(cast for float) into result, allowing NaN/Infinity to propagate the same way as
the regular conversion/error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b22cd0a8-9e8d-451d-9638-2f39eb1384a8
📒 Files selected for processing (109)
.gitignoreNativeScript/CMakeLists.txtNativeScript/cli/main.cppNativeScript/ffi/hermes/HermesFastCallbackInfo.hNativeScript/ffi/hermes/HermesFastConversion.mmNativeScript/ffi/hermes/HermesFastNativeApi.hNativeScript/ffi/hermes/HermesFastNativeApi.mmNativeScript/ffi/hermes/HermesFastNativeApiPrivate.hNativeScript/ffi/hermes/jsi/NativeApiJsi.hNativeScript/ffi/hermes/jsi/NativeApiJsi.mmNativeScript/ffi/hermes/jsi/NativeApiJsiReactNative.hNativeScript/ffi/hermes/jsi/README.mdNativeScript/ffi/jsc/JSCFastConversion.mmNativeScript/ffi/jsc/JSCFastNativeApi.hNativeScript/ffi/jsc/JSCFastNativeApi.mmNativeScript/ffi/jsc/JSCFastNativeApiPrivate.hNativeScript/ffi/napi/AutoreleasePool.hNativeScript/ffi/napi/AutoreleasePool.mmNativeScript/ffi/napi/Block.hNativeScript/ffi/napi/Block.mmNativeScript/ffi/napi/CFunction.hNativeScript/ffi/napi/CFunction.mmNativeScript/ffi/napi/CallbackThreading.hNativeScript/ffi/napi/Cif.hNativeScript/ffi/napi/Cif.mmNativeScript/ffi/napi/Class.hNativeScript/ffi/napi/Class.mmNativeScript/ffi/napi/ClassBuilder.hNativeScript/ffi/napi/ClassBuilder.mmNativeScript/ffi/napi/ClassMember.hNativeScript/ffi/napi/ClassMember.mmNativeScript/ffi/napi/Closure.hNativeScript/ffi/napi/Closure.mmNativeScript/ffi/napi/Enum.hNativeScript/ffi/napi/Enum.mmNativeScript/ffi/napi/InlineFunctions.hNativeScript/ffi/napi/InlineFunctions.mmNativeScript/ffi/napi/Interop.hNativeScript/ffi/napi/Interop.mmNativeScript/ffi/napi/JSObject.hNativeScript/ffi/napi/JSObject.mmNativeScript/ffi/napi/NativeScriptException.hNativeScript/ffi/napi/NativeScriptException.mmNativeScript/ffi/napi/ObjCBridge.hNativeScript/ffi/napi/ObjCBridge.mmNativeScript/ffi/napi/Object.hNativeScript/ffi/napi/Object.mmNativeScript/ffi/napi/ObjectRef.hNativeScript/ffi/napi/ObjectRef.mmNativeScript/ffi/napi/Protocol.hNativeScript/ffi/napi/Protocol.mmNativeScript/ffi/napi/Struct.hNativeScript/ffi/napi/Struct.mmNativeScript/ffi/napi/TypeConv.hNativeScript/ffi/napi/TypeConv.mmNativeScript/ffi/napi/Util.hNativeScript/ffi/napi/Util.mmNativeScript/ffi/napi/Variable.hNativeScript/ffi/napi/Variable.mmNativeScript/ffi/napi/node_api_util.hNativeScript/ffi/quickjs/QuickJSFastConversion.mmNativeScript/ffi/quickjs/QuickJSFastNativeApi.hNativeScript/ffi/quickjs/QuickJSFastNativeApi.mmNativeScript/ffi/quickjs/QuickJSFastNativeApiPrivate.hNativeScript/ffi/quickjs/QuickJSFastReturn.mmNativeScript/ffi/shared/EngineDirectCall.hNativeScript/ffi/shared/EngineDirectCall.mmNativeScript/ffi/shared/InvocationSupport.hNativeScript/ffi/shared/SignatureDispatch.hNativeScript/ffi/shared/Tasks.cppNativeScript/ffi/shared/Tasks.hNativeScript/ffi/v8/V8FastConversion.mmNativeScript/ffi/v8/V8FastNativeApi.hNativeScript/ffi/v8/V8FastNativeApi.mmNativeScript/ffi/v8/V8FastNativeApiPrivate.hNativeScript/ffi/v8/V8FastNativeWrapper.mmNativeScript/napi/jsc/jsc-api.cppNativeScript/napi/quickjs/quickjs-api.cNativeScript/napi/v8/v8-api.cppNativeScript/napi/v8/v8_inspector/Utils.cppNativeScript/napi/v8/v8_inspector/ns-v8-tracing-agent-impl.cppNativeScript/runtime/NativeScript.mmNativeScript/runtime/Runtime.cppNativeScript/runtime/Util.hNativeScript/runtime/modules/console/Console.cppNativeScript/runtime/modules/module/ModuleInternal.cppNativeScript/runtime/modules/timers/Timers.mmNativeScript/runtime/modules/worker/MessageV8.cppNativeScript/runtime/modules/worker/Worker.mmNativeScript/runtime/modules/worker/WorkerImpl.hNativeScript/runtime/modules/worker/WorkerImpl.mmmetadata-generator/CMakeLists.txtmetadata-generator/build-step-metadata-generator.pymetadata-generator/src/SignatureDispatchEmitter.cppmetadata-generator/src/SignatureDispatchEmitter/EngineDirect.cppmetadata-generator/src/SignatureDispatchEmitter/Hermes.cppmetadata-generator/src/SignatureDispatchEmitter/Napi.cppmetadata-generator/src/SignatureDispatchEmitter/Shared.cppmetadata-generator/src/SignatureDispatchEmitter/Shared.hmetadata-generator/src/SignatureDispatchEmitter/V8.cpppackages/react-native/src/index.tsscripts/build_nativescript.shscripts/build_react_native_turbomodule.shscripts/metagen.jsscripts/react_native_app_utils.shscripts/run-tests-macos.jsscripts/test_react_native_ffi_compat.shtest/react-native/ffi-compat/App.tsxtest/runtime/fixtures/TestFixtures.h
💤 Files with no reviewable changes (25)
- NativeScript/ffi/quickjs/QuickJSFastNativeApi.h
- NativeScript/ffi/napi/Object.h
- NativeScript/ffi/napi/Cif.h
- NativeScript/ffi/v8/V8FastNativeApi.h
- NativeScript/ffi/hermes/jsi/README.md
- NativeScript/ffi/jsc/JSCFastNativeApi.h
- NativeScript/ffi/shared/EngineDirectCall.h
- NativeScript/ffi/napi/Interop.mm
- NativeScript/ffi/hermes/HermesFastNativeApi.h
- NativeScript/ffi/hermes/jsi/NativeApiJsi.h
- NativeScript/ffi/napi/Class.mm
- NativeScript/ffi/napi/Protocol.mm
- NativeScript/ffi/napi/Block.h
- NativeScript/ffi/hermes/HermesFastCallbackInfo.h
- NativeScript/ffi/napi/CFunction.h
- NativeScript/ffi/napi/ObjCBridge.mm
- NativeScript/ffi/hermes/jsi/NativeApiJsiReactNative.h
- NativeScript/ffi/napi/CallbackThreading.h
- NativeScript/ffi/napi/ClassMember.h
- NativeScript/ffi/napi/ClassBuilder.mm
- NativeScript/ffi/napi/Cif.mm
- NativeScript/ffi/napi/Object.mm
- NativeScript/ffi/napi/TypeConv.h
- NativeScript/ffi/napi/Block.mm
- NativeScript/ffi/napi/ObjCBridge.h
✅ Files skipped from review due to trivial changes (6)
- NativeScript/napi/v8/v8_inspector/ns-v8-tracing-agent-impl.cpp
- NativeScript/napi/v8/v8_inspector/Utils.cpp
- NativeScript/cli/main.cpp
- NativeScript/runtime/modules/worker/WorkerImpl.h
- NativeScript/runtime/modules/module/ModuleInternal.cpp
- NativeScript/runtime/Runtime.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
Benchmark Results
Command family:
node benchmarks/objc-dispatch/run.js --iterations 250000using the same simulator/harness for all rows.GSD deltas:
Result files:
build/benchmarks/objc-dispatch/results-2026-05-22T19-44-10-380Z.jsonbuild/benchmarks/objc-dispatch/results-2026-05-22T19-46-55-756Z.jsonbuild/benchmarks/objc-dispatch/results-2026-05-22T19-48-29-266Z.jsonbuild/benchmarks/objc-dispatch/results-2026-05-22T19-49-26-650Z.jsonbuild/benchmarks/objc-dispatch/results-2026-05-22T19-50-11-344Z.jsonValidation
git diff --check origin/main..HEADIOS_TEST_ENGINE=hermes IOS_TEST_TIMEOUT_MS=900000 IOS_BUILD_TIMEOUT_MS=900000 npm run test:iosSUCCESS: 705 specs, 0 failures, 14 skipped, 0 disabledReview Notes
Summary by CodeRabbit
Release Notes
New Features
Documentation
Build & Tooling